-
Notifications
You must be signed in to change notification settings - Fork 12k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WebAssembly] Disable reference types in generic CPU #90792
Conversation
llvm#80923 newly enabled multivalue and reference-types in the generic CPU. But enabling reference-types ended up breaking up Wasm's Chromium CI (https://chromium-review.googlesource.com/c/emscripten-releases/+/5500231) because the way the table index is encoded different from MVP (u32) vs. reference-types (LEB), which caused different encodings for `call_indirect`. And Chromium CI's and Emscripten's minimum required node version is v16, which does not yet support reference-types, which does not recognize that table index encoding. reference-types is first supported in node v17.2. We knew the current minimum required node for Emscripten (v16) did not support reference-types, but thought it was fine because unless you explicitly use `__funcref` or `__externref` things would be fine, and if you want to use them explicitly, you would have a newer node. But it turned out it also affected the encoding of `call_indirect`. While we are discussing the potential solutions, I will disable reference-types to unblock the rolls.
@llvm/pr-subscribers-backend-webassembly @llvm/pr-subscribers-clang Author: Heejin Ahn (aheejin) Changes#80923 newly enabled multivalue and reference-types in the generic CPU. And Chromium CI's and Emscripten's minimum required node version is v16, which does not yet support reference-types, which does not recognize that table index encoding. reference-types is first supported in node v17.2. We knew the current minimum required node for Emscripten (v16) did not support reference-types, but thought it was fine because unless you explicitly use While we are discussing the potential solutions, I will disable reference-types to unblock the rolls. Full diff: https://github.com/llvm/llvm-project/pull/90792.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2c5308fbcb319a..0e3f7cf89ca885 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -742,10 +742,10 @@ AIX Support
WebAssembly Support
^^^^^^^^^^^^^^^^^^^
-The -mcpu=generic configuration now enables multivalue and reference-types.These
-proposals are standardized and available in all major engines. Enabling
-multivalue here only enables the language feature but does not turn on the
-multivalue ABI (this enables non-ABI uses of multivalue, like exnref).
+The -mcpu=generic configuration now enables multivalue feature, which is
+standardized and available in all major engines. Enabling multivalue here only
+enables the language feature but does not turn on the multivalue ABI (this
+enables non-ABI uses of multivalue, like exnref).
AVR Support
^^^^^^^^^^^
diff --git a/clang/lib/Basic/Targets/WebAssembly.cpp b/clang/lib/Basic/Targets/WebAssembly.cpp
index 1f0418b21c1f86..a6d820e108088f 100644
--- a/clang/lib/Basic/Targets/WebAssembly.cpp
+++ b/clang/lib/Basic/Targets/WebAssembly.cpp
@@ -153,7 +153,6 @@ bool WebAssemblyTargetInfo::initFeatureMap(
auto addGenericFeatures = [&]() {
Features["multivalue"] = true;
Features["mutable-globals"] = true;
- Features["reference-types"] = true;
Features["sign-ext"] = true;
};
auto addBleedingEdgeFeatures = [&]() {
@@ -162,6 +161,7 @@ bool WebAssemblyTargetInfo::initFeatureMap(
Features["bulk-memory"] = true;
Features["multimemory"] = true;
Features["nontrapping-fptoint"] = true;
+ Features["reference-types"] = true;
Features["tail-call"] = true;
Features["half-precision"] = true;
setSIMDLevel(Features, SIMD128, true);
diff --git a/clang/test/Preprocessor/wasm-target-features.c b/clang/test/Preprocessor/wasm-target-features.c
index 72ecc60a6e7898..5a4f85461d5aa2 100644
--- a/clang/test/Preprocessor/wasm-target-features.c
+++ b/clang/test/Preprocessor/wasm-target-features.c
@@ -164,7 +164,6 @@
//
// GENERIC-INCLUDE-DAG: #define __wasm_multivalue__ 1{{$}}
// GENERIC-INCLUDE-DAG: #define __wasm_mutable_globals__ 1{{$}}
-// GENERIC-INCLUDE-DAG: #define __wasm_reference_types__ 1{{$}}
// GENERIC-INCLUDE-DAG: #define __wasm_sign_ext__ 1{{$}}
//
// RUN: %clang -E -dM %s -o - 2>&1 \
@@ -181,6 +180,7 @@
// GENERIC-NOT: #define __wasm_half_precision__ 1{{$}}
// GENERIC-NOT: #define __wasm_multimemory__ 1{{$}}
// GENERIC-NOT: #define __wasm_nontrapping_fptoint__ 1{{$}}
+// GENERIC-NOT: #define __wasm_reference_types__ 1{{$}}
// GENERIC-NOT: #define __wasm_relaxed_simd__ 1{{$}}
// GENERIC-NOT: #define __wasm_simd128__ 1{{$}}
// GENERIC-NOT: #define __wasm_tail_call__ 1{{$}}
|
Will land this given that this is necessary to unblock the rolls and the full CI will take more than a full day. |
I submitted emscripten-core#21853 before landing llvm/llvm-project#80923, so the previous version (3.1.59) was released before the LLVM change actually landed. And then we decided to disable reference-types temporarily (llvm/llvm-project#90792) while the node version is being resolved (emscripten-core/emsdk#1173 (comment)). This moves the entry to the current release and only mentions multivalue is the newly enabled feature.
I submitted #21853 before landing llvm/llvm-project#80923, so the previous version (3.1.59) was released before the LLVM change actually landed. And then we decided to disable reference-types temporarily (llvm/llvm-project#90792) while the node version is being resolved (emscripten-core/emsdk#1173 (comment)). This moves the entry to the current release and only mentions multivalue is the newly enabled feature.
Now that we are about to upgrade emsdk's default node to v18.20.3 (emscripten-core/emsdk#1387), we can re-enable reference-types by default again. This effectively reverts llvm#90792.
Now that we are about to upgrade emsdk's default node to v18.20.3 (emscripten-core/emsdk#1387), we can re-enable reference-types by default again. This effectively reverts #90792.
Now that we are about to upgrade emsdk's default node to v18.20.3 (emscripten-core/emsdk#1387), we can re-enable reference-types by default again. This effectively reverts llvm#90792.
#80923 newly enabled multivalue and reference-types in the generic CPU.
But enabling reference-types ended up breaking up Wasm's Chromium CI (https://chromium-review.googlesource.com/c/emscripten-releases/+/5500231) because the way the table index is encoded is different from MVP (u32) vs. reference-types (LEB), which caused different encodings for
call_indirect
.And Chromium CI's and Emscripten's minimum required node version is v16, which does not yet support reference-types, which does not recognize that table index encoding. reference-types is first supported in node v17.2.
We knew the current minimum required node for Emscripten (v16) did not support reference-types, but thought it was fine because unless you explicitly use
__funcref
or__externref
things would be fine, and if you want to use them explicitly, you would have a newer node. But it turned out it also affected the encoding ofcall_indirect
.While we are discussing the potential solutions, I will disable reference-types to unblock the rolls.